PMM-14858 Skip built-in ClickHouse scrape for external instances#5369
PMM-14858 Skip built-in ClickHouse scrape for external instances#53694nte wants to merge 6 commits into
Conversation
4nte
requested review from
JiriCtvrtka and
maxkondr
and removed request for
a team
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3 #5369 +/- ##
==========================================
+ Coverage 43.18% 43.25% +0.07%
==========================================
Files 413 413
Lines 42253 42258 +5
==========================================
+ Hits 18247 18280 +33
+ Misses 22140 22112 -28
Partials 1866 1866
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
| clickhouseAddrF := kingpin.Flag("clickhouse-addr", "Clickhouse database address").Default("127.0.0.1:9000").Envar("PMM_CLICKHOUSE_ADDR").String() | ||
| clickhouseUsernameF := kingpin.Flag("clickhouse-username", "Clickhouse database user").Default("default").Envar("PMM_CLICKHOUSE_USER").String() | ||
| clickhousePasswordF := kingpin.Flag("clickhouse-password", "Clickhouse database user password").Default("clickhouse").Envar("PMM_CLICKHOUSE_PASSWORD").String() | ||
| clickhouseBuiltinDisabledF := kingpin.Flag("clickhouse-disable-builtin", "Disable the built-in ClickHouse").Envar("PMM_DISABLE_BUILTIN_CLICKHOUSE").Bool() |
There was a problem hiding this comment.
I do not see any benefit from this additional var. The fact of set PMM_CLICKHOUSE_ADDR looks enough that external CH usage is requested.
There was a problem hiding this comment.
I was considering following options:
- Check if
PMM_CLICKHOUSE_ADDRhostname is not127.0.0.1 - Check if
PMM_DISABLE_BUILTIN_CLICKHOUSEis set - Check both 1. and 2.
I agree PMM_CLICKHOUSE_ADDR is sufficient to determine this and would prefer it too. My concern is that PMM_DISABLE_BUILTIN_CLICKHOUSE is already explicitly documented as the flag that disables internal CH and managed-init already relies on it.
Not checking it here, while checking it in managed-init seemed inconsistent, unless we treat PMM_DISABLE_BUILTIN_CLICKHOUSE as an env var that is exclusive to managed-init.
Having all that said, do you still think we should just infer from PMM_CLICKHOUSE_ADDR and not lookup PMM_DISABLE_BUILTIN_CLICKHOUSE?
There was a problem hiding this comment.
We agreed to deprecate all PMM_DISABLE_BUILTIN_ env variables, since their use conflicts with PMM_<TECH>_ADDR variables, which should be the only source of truth. Therefore, this fix will rely on PMM_CLICKHOUSE_ADDR.
There was a problem hiding this comment.
I've also discussed this with @ademidoff, we all agree PMM_CLICKHOUSE_ADDR is technically sufficient to determine whether CH is internal or external.
I will omit PMM_DISABLE_BUILTIN_CLICKHOUSE and rely on PMM_CLICKHOUSE_ADDR as a single source of truth.
Since PMM_DISABLE_BUILTIN_CLICKHOUSE is obviously redundant and creates ambiguity, I will create a separate issue to deprecate PMM_DISABLE_BUILTIN_CLICKHOUSE in favor of inferring from PMM_CLICKHOUSE_ADDR
Co-authored-by: Maxim Kondratenko <maxim.kondratenko@percona.com>
5 participants
PMM-14858
Feature build
PMM kept scraping
127.0.0.1:9363even when an external ClickHouse was configured orPMM_DISABLE_BUILTIN_CLICKHOUSEwas set.Introduced new
ClickHouseParamsmodel struct that validates and facilitates CH parameters.ClickHouse scraping is skipped when any of the following are true:
PMM_DISABLE_BUILTIN_CLICKHOUSEis set. (new)PMM_CLICKHOUSE_ADDRpoints to an external host. (new)Addditionally, we log a warning when
PMM_DISABLE_BUILTIN_CLICKHOUSEis not set andPMM_CLICKHOUSE_ADDRis an external address (not127.0.0.1) to warn user about unwanted drift in their configuration.